-
Notifications
You must be signed in to change notification settings - Fork 245
feat: add cloudtrail tracking to execute_bash #2535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
#[test] | ||
fn test_cloudtrail_tracking_with_existing_env() { | ||
// Set an existing AWS_EXECUTION_ENV value | ||
unsafe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not use unsafe - tests run in multiple threads within the same process. Instead, pass the Os
type for getting and setting env vars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we still going to keep the unsafe
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is more context here. According to him unsafe here is not unsafe. I had added a non unsafe version but that was very complicated. So he referred to this method.
} | ||
|
||
#[tokio::test] | ||
async fn test_cloudtrail_tracking_with_existing_env() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't construct Os
like this, you could just do Os::new
then follow with an unsafe set_var
(note that this is safe because the test instance uses an in-memory hashmap)
@@ -418,6 +421,36 @@ pub fn queue_function_result(result: &str, updates: &mut impl Write, is_error: b | |||
Ok(()) | |||
} | |||
|
|||
/// Helper function to set up environment variables with user agent metadata for CloudTrail tracking | |||
pub fn user_agent_env_vars(os: &Os) -> std::collections::HashMap<String, String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is including all env vars, probably better to instead call it env_vars_with_user_agent
- Add CloudTrail user agent metadata to execute_bash tool for tracking - Consolidate CloudTrail constants in consts.rs to eliminate duplication - Create shared env_vars_with_user_agent() function used by both use_aws and execute_bash - Add comprehensive tests for CloudTrail tracking functionality - Support both Unix and Windows platforms with proper environment variable handling - Maintain backward compatibility and thread-safe implementation
Issue #, if available:
Cloudtrail logs were not having QCLI identifier if we used execute_bash tool.
Description of changes:
Added the code to enable these identifires.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.